Conversation
If serial buffer contained more values than requested by erpc underlying receive `serial_read` got stuck.
| ClearCommError(hCom, &errors, NULL); | ||
|
|
||
| if (!ReadFile(hCom, temp, RX_BUF_BYTES - bytesToRead, &bytesRead, &s_readOverlap)) | ||
| if (!ReadFile(hCom, temp, size - totalBytesRead, &bytesRead, &s_readOverlap)) |
There was a problem hiding this comment.
It seems like what the original code was trying to do is read size bytes and size may be larger than RX_BUF_BYTES. When size > RX_BUF_BYTES it is necessary to call ReadFile multiple times. It seems to me like the bytesToRead variable is named badly. I don't really know the details of the Windows serial API, but to me, it seems like the overall flow should be something like this:
char temp[RX_BUF_BYTES];
size_t totalBytesRead = 0;
while (totalBytesRead < size) {
const DWORD bytesToRead = std::min(RX_BUF_BYTES, size - totalBytesRead);
DWORD bytesRead;
if (!ReadFile(hCom, temp, bytesToRead, &bytesRead, &s_readOverlap)) {
// TODO: error handling
}
if (bytesRead == 0) {
// TODO: can this happen?
}
memcpy(&buf[totalBytesRead], temp, bytesRead);
totalBytesRead += bytesRead;
}
return totalBytesRead;
There was a problem hiding this comment.
The problem with the original implementation was that:
if (!ReadFile(hCom, temp, RX_BUF_BYTES - bytesToRead, &bytesRead, &s_readOverlap))
leads to reading all bytes which were in the RX buffer. However, erpc tries to read at first 4 bytes then should return and tries to read the remaining bytes in another call to serial_read. So the original implementation got stuck when it read more than 4 bytes because this loop:
while (bytesToRead != size)
was never exited due to bytesToRead (naming is really confusing, so I changed it) got bigger than size .
Changing the implementation to
if (!ReadFile(hCom, temp, size - totalBytesRead, &bytesRead, &s_readOverlap))
ensures that only the desired amount of data is being retrieved from the buffer.
Cosnidering you proposal:
if (bytesRead == 0) {
// TODO: can this happen?
}
yes this can happen, and the loop should continue until it has read the desired amount of data.
For this snippet:
if (!ReadFile(hCom, temp, bytesToRead, &bytesRead, &s_readOverlap)) {
// TODO: error handling
}
My aim wasn't to improve error handling and therefore would keep what is implemented, i.e.
{
if (GetLastError() == ERROR_IO_PENDING)
{
if (!GetOverlappedResult(hCom, &s_readOverlap, &bytesRead, FALSE))
{
bytesRead = 0;
totalBytesRead = 0;
}
}
else
{
bytesRead = 0;
totalBytesRead = 0;
}
}
else
{
bytesRead = 0;
totalBytesRead = 0;
}
}
|
Thank you @aelray for providing this PR and the detailed description. These changes seems to be reasonable but the internal testing for the v1.8.1 release is almost finished, I would rather integrate this PR after the release, i.e. in about 3w, ok? Thank you. |
|
Hi @aelray , as see in Copyright we are not authors of this file. I actually don't know from where this file was took. Any chance to found original code based on Copyright and see their fix if any was done? |
|
There is the same issue ReadFile |
5 participants
If serial buffer contained more values than requested by erpc
underlying_receiveserial_readgot stuck.